CA2241 Provide correct arguments to formatting methods #7190
Conversation
|
|
||
| UncPattern.IsMatch(winDirectory).ShouldBe(false); | ||
| UncPattern.IsMatch(unixDirectory).ShouldBe(false); | ||
| UncPattern.IsMatch(string.Empty).ShouldBe(false); |
There was a problem hiding this comment.
Looking at all these tests, they don't really feel substantively different from each other, and many check the same thing twice. Maybe condense into one test?
There was a problem hiding this comment.
How are those changes?
There was a problem hiding this comment.
@Forgind I disagree pretty strongly with this recommendation, because it violates the test principle that a test should fail for only one reason. Here the new test could fail if one of the implementations fails but it wouldn't be super clear from the failure which one.
@elachlan don't bother reverting though; this isn't a huge deal and you already did the unification.
There was a problem hiding this comment.
That's fair. I was borderline on suggesting just removing the legacy tests entirely—they test Regex, which isn't even in MSBuild. FileUtilitiesRegex.IsUncPattern and FileUtilitiesRegex.StartsWithUncPattern both just call FileUtilitiesRegex.StartsWithUncPatternMatchLength, so it all felt like one test to me. It's possible that won't be true in the future, though.
There was a problem hiding this comment.
Up to the team, I don't mind either way. Just let me know what you want me to do. If there isn't any changes then is this okay to merge?
Forgind
left a comment
Forgind
left a comment
There was a problem hiding this comment.
Looks about right to me 🙂
|
|
||
| UncPattern.IsMatch(winDirectory).ShouldBe(false); | ||
| UncPattern.IsMatch(unixDirectory).ShouldBe(false); | ||
| UncPattern.IsMatch(string.Empty).ShouldBe(false); |
There was a problem hiding this comment.
@Forgind I disagree pretty strongly with this recommendation, because it violates the test principle that a test should fail for only one reason. Here the new test could fail if one of the implementations fails but it wouldn't be super clear from the failure which one.
@elachlan don't bother reverting though; this isn't a huge deal and you already did the unification.
ladipro
added
the
merge-when-branch-open
4 participants
Relates to #7174